refactor(visualizeNetworks): Refactor exportNetworktoHTML and previewNetwork to use cytoscapeNetwork function#73
Merged
tonywu1999 merged 7 commits intodevelfrom Mar 1, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the exported Changes
Sequence Diagram(s)sequenceDiagram
participant User as Caller
participant Rfunc as exportNetworkToHTML / previewNetworkInBrowser
participant Widget as cytoscapeNetwork widget
participant HW as htmlwidgets::saveWidget
participant Browser as Browser / File
User->>Rfunc: call exportNetworkToHTML(nodes, edges, ...)
Rfunc->>Widget: construct cytoscapeNetwork widget (elements, layout, options)
Widget->>HW: saveWidget(widget, file, selfcontained=TRUE)
HW->>Browser: write standalone HTML file
HW-->>Rfunc: return filepath
Rfunc-->>User: filepath (and optionally open via browseURL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
R/visualizeNetworksWithHTML.R (2)
39-39:⚠️ Potential issue | 🟡 MinorSame stale documentation and unused
...parameter.Consistent with
exportNetworkToHTML, the...parameter is documented as being passed toexportCytoscapeToHTML()(which was removed), and is not passed to theexportNetworkToHTML()call on lines 49-52.🔧 Proposed fix to remove unused parameter
-#' `@param` ... Additional arguments passed to exportCytoscapeToHTML() previewNetworkInBrowser <- function(nodes, edges, displayLabelType = "id", - nodeFontSize = 12, - ...) { + nodeFontSize = 12) {Also applies to: 43-43, 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` at line 39, Remove the stale ... parameter and its roxygen `@param` documentation from the visualizeNetworksWithHTML.R function (the documented reference to exportCytoscapeToHTML which no longer exists), and ensure calls to exportNetworkToHTML(...) do not expect or forward ...; update the function signature to drop ..., remove the corresponding `@param` ... doc entry, and clean any internal references that attempted to pass ... to exportNetworkToHTML so the function and its docs match the actual call pattern.
11-11:⚠️ Potential issue | 🟡 MinorStale documentation and unused
...parameter.The
@param ...documentation referencesexportCytoscapeToHTML()which was removed in this refactor. The...parameter is declared but never passed to any function call (cytoscapeNetworkorhtmlwidgets::saveWidget).Either remove the
...parameter entirely or pass it to the underlying functions if additional arguments should be supported.🔧 Proposed fix to remove unused parameter
-#' `@param` ... Additional arguments passed to exportCytoscapeToHTML() #' `@export` #' `@return` Invisibly returns the file path of the created HTML file exportNetworkToHTML <- function(nodes, edges, filename = "network_visualization.html", displayLabelType = "id", - nodeFontSize = 12, - ...) { + nodeFontSize = 12) {Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` at line 11, The roxygen `@param` ... and the unused dots parameter in visualizeNetworksWithHTML are stale (exportCytoscapeToHTML was removed) and not forwarded to cytoscapeNetwork or htmlwidgets::saveWidget; remove the ... parameter from the visualizeNetworksWithHTML function signature and delete its `@param` ... doc entry (and any other references to it) so the docs and signature match actual usage, or alternatively explicitly forward ... to cytoscapeNetwork and/or htmlwidgets::saveWidget if you want to support extra args (update the roxygen accordingly); locate the function visualizeNetworksWithHTML and the related calls to cytoscapeNetwork and htmlwidgets::saveWidget to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/visualizeNetworksWithHTML.R`:
- Around line 20-28: The function visualizeNetworksWithHTML is missing the
promised invisible return of the created file path and still declares an unused
... parameter; update the function to return(invisible(filename)) after calling
htmlwidgets::saveWidget (use the existing widget and filename symbols) so
callers receive the file path, and remove or repurpose the ... parameter (and
update previewNetworkInBrowser similarly) so that ... is either deleted from the
function signature and docs or wired to actual downstream calls if additional
args are needed; adjust the roxygen `@return` and `@param` ... docs to match the
final behavior.
---
Outside diff comments:
In `@R/visualizeNetworksWithHTML.R`:
- Line 39: Remove the stale ... parameter and its roxygen `@param` documentation
from the visualizeNetworksWithHTML.R function (the documented reference to
exportCytoscapeToHTML which no longer exists), and ensure calls to
exportNetworkToHTML(...) do not expect or forward ...; update the function
signature to drop ..., remove the corresponding `@param` ... doc entry, and clean
any internal references that attempted to pass ... to exportNetworkToHTML so the
function and its docs match the actual call pattern.
- Line 11: The roxygen `@param` ... and the unused dots parameter in
visualizeNetworksWithHTML are stale (exportCytoscapeToHTML was removed) and not
forwarded to cytoscapeNetwork or htmlwidgets::saveWidget; remove the ...
parameter from the visualizeNetworksWithHTML function signature and delete its
`@param` ... doc entry (and any other references to it) so the docs and signature
match actual usage, or alternatively explicitly forward ... to cytoscapeNetwork
and/or htmlwidgets::saveWidget if you want to support extra args (update the
roxygen accordingly); locate the function visualizeNetworksWithHTML and the
related calls to cytoscapeNetwork and htmlwidgets::saveWidget to apply the
change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
NAMESPACER/visualizeNetworksWithHTML.Rman/generateCytoscapeConfig.Rdman/generateJavaScriptCode.Rd
💤 Files with no reviewable changes (3)
- NAMESPACE
- man/generateJavaScriptCode.Rd
- man/generateCytoscapeConfig.Rd
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #73 +/- ##
===========================================
+ Coverage 50.32% 78.66% +28.34%
===========================================
Files 8 7 -1
Lines 1856 825 -1031
===========================================
- Hits 934 649 -285
+ Misses 922 176 -746 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/testthat/test-visualizeNetworksWithHTML.R (2)
220-220: Inconsistent empty edges pattern.Several tests pass
data.frame()(a data frame with zero columns) as the edges argument, while elsewhere in the file (lines 168-169, 199-200, 312-313) empty edges are created with explicit column structure (source,target,interaction).If
.buildElementschecks for specific columns on edges, the no-column variant could behave differently. Consider using a consistent pattern throughout:♻️ Suggested consistent empty edges pattern
test_that(".buildElements assigns correct node_type to proteins", { nodes <- create_mock_nodes() - result <- MSstatsBioNet:::.buildElements(nodes, data.frame()) + empty_edges <- data.frame(source = character(0), target = character(0), + interaction = character(0), stringsAsFactors = FALSE) + result <- MSstatsBioNet:::.buildElements(nodes, empty_edges)Or create a reusable helper:
create_empty_edges <- function() { data.frame(source = character(0), target = character(0), interaction = character(0), stringsAsFactors = FALSE) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/testthat/test-visualizeNetworksWithHTML.R` at line 220, Tests call MSstatsBioNet:::.buildElements(nodes, data.frame()) which creates an inconsistent empty-edges shape compared to other tests that pass a data.frame with columns source, target, interaction; update this test to pass an empty edges data.frame that includes the expected columns (source, target, interaction) with zero rows (or replace all tests with a shared helper create_empty_edges()) so .buildElements sees a consistent schema when validating edge columns and avoids differing behavior between the no-column and typed-empty variants.
322-324: Test assertion could be more specific.The regex
"id column|data frame"matches either error message, which could mask if the function produces a different (potentially worse) error message than expected. Consider testing more specifically:♻️ More precise error assertion
test_that("cytoscapeNetwork() errors when nodes is not a data frame", { - expect_error(cytoscapeNetwork(list(id = "A")), "id column|data frame") + expect_error(cytoscapeNetwork(list(id = "A")), "data frame", ignore.case = TRUE) })Alternatively, if the exact error message is known, use the full expected text for stricter validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/testthat/test-visualizeNetworksWithHTML.R` around lines 322 - 324, The test's error assertion is too loose; update the test for cytoscapeNetwork to assert a specific error message about the nodes argument being a data frame. Replace the current expect_error(cytoscapeNetwork(list(id = "A")), "id column|data frame") with a stricter assertion that matches the exact or a more precise regexp such as one that states the nodes argument must be a data.frame (e.g. "nodes.*data.frame" or the exact error text your cytoscapeNetwork implementation throws) so the test fails only when the expected validation message changes; refer to the cytoscapeNetwork function to pick the exact message to assert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/testthat/test-visualizeNetworksWithHTML.R`:
- Line 220: Tests call MSstatsBioNet:::.buildElements(nodes, data.frame()) which
creates an inconsistent empty-edges shape compared to other tests that pass a
data.frame with columns source, target, interaction; update this test to pass an
empty edges data.frame that includes the expected columns (source, target,
interaction) with zero rows (or replace all tests with a shared helper
create_empty_edges()) so .buildElements sees a consistent schema when validating
edge columns and avoids differing behavior between the no-column and typed-empty
variants.
- Around line 322-324: The test's error assertion is too loose; update the test
for cytoscapeNetwork to assert a specific error message about the nodes argument
being a data frame. Replace the current expect_error(cytoscapeNetwork(list(id =
"A")), "id column|data frame") with a stricter assertion that matches the exact
or a more precise regexp such as one that states the nodes argument must be a
data.frame (e.g. "nodes.*data.frame" or the exact error text your
cytoscapeNetwork implementation throws) so the test fails only when the expected
validation message changes; refer to the cytoscapeNetwork function to pick the
exact message to assert.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
R/visualizeNetworksWithHTML.R (1)
8-26:⚠️ Potential issue | 🟠 MajorReturn contract and
...handling are still inconsistent.Line 10 documents an invisible filename return, but the function currently returns
NULL.
Also,...still references a removed function and is never forwarded, so extra caller arguments are silently ignored.🐛 Proposed fix
-#' `@param` ... Additional arguments passed to exportCytoscapeToHTML() +#' `@param` ... Additional arguments passed to \code{cytoscapeNetwork()}. @@ exportNetworkToHTML <- function(nodes, edges, filename = "network_visualization.html", displayLabelType = "id", nodeFontSize = 12, ...) { widget <- cytoscapeNetwork(nodes, edges, displayLabelType = displayLabelType, - nodeFontSize = nodeFontSize) + nodeFontSize = nodeFontSize, + ...) @@ htmlwidgets::saveWidget( widget, file = filename, selfcontained = TRUE ) + + invisible(filename) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/visualizeNetworksWithHTML.R` around lines 8 - 26, The function exportNetworkToHTML currently drops any ... args and returns NULL despite the docs; update exportNetworkToHTML to forward ... into htmlwidgets::saveWidget (so extra caller arguments are not ignored) while keeping the widget creation via cytoscapeNetwork(...), and at the end invisibly return the filename (use invisible(filename)); ensure the roxygen `@param` ... and `@return` docs match this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@R/visualizeNetworksWithHTML.R`:
- Around line 8-26: The function exportNetworkToHTML currently drops any ...
args and returns NULL despite the docs; update exportNetworkToHTML to forward
... into htmlwidgets::saveWidget (so extra caller arguments are not ignored)
while keeping the widget creation via cytoscapeNetwork(...), and at the end
invisibly return the filename (use invisible(filename)); ensure the roxygen
`@param` ... and `@return` docs match this behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
DESCRIPTIONNAMESPACER/cytoscapeNetwork.RR/utils_getSubnetworkFromIndra.RR/visualizeNetworksWithHTML.Rman/exportNetworkToHTML.Rdman/previewNetworkInBrowser.Rdvignettes/Cytoscape-Visualization.Rmdvignettes/PTM-Analysis.Rmd
✅ Files skipped from review due to trivial changes (4)
- man/exportNetworkToHTML.Rd
- vignettes/Cytoscape-Visualization.Rmd
- man/previewNetworkInBrowser.Rd
- vignettes/PTM-Analysis.Rmd
🚧 Files skipped from review as they are similar to previous changes (1)
- NAMESPACE
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
R/exportNetworkToHTML.R (1)
8-15:⚠️ Potential issue | 🟡 MinorRemove or repurpose the unused
...and stale roxygen text.Line 8 documents forwarding to
exportCytoscapeToHTML(), but that function is no longer used, and Line 15 accepts...without forwarding it anywhere. This silently ignores extra arguments.♻️ Suggested cleanup
-#' `@param` ... Additional arguments passed to exportCytoscapeToHTML() exportNetworkToHTML <- function(nodes, edges, filename = "network_visualization.html", displayLabelType = "id", - nodeFontSize = 12, - ...) { + nodeFontSize = 12) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/exportNetworkToHTML.R` around lines 8 - 15, The function exportNetworkToHTML currently declares an unused ... argument and has a stale roxygen `@param` ... description mentioning exportCytoscapeToHTML; remove the unused ... from the function signature and delete or update the roxygen line that reads "Additional arguments passed to exportCytoscapeToHTML()", ensuring the documentation accurately reflects the actual parameters (e.g., nodes, edges, filename, displayLabelType, nodeFontSize) and return value; keep the `@export` tag and verify no other internal calls rely on ... in exportNetworkToHTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@R/exportNetworkToHTML.R`:
- Around line 11-56: Add testthat coverage for exportNetworkToHTML and
previewNetworkInBrowser by writing tests that (1) assert htmlwidgets::saveWidget
is called with the provided filename when exportNetworkToHTML(nodes, edges,
filename=...) is invoked (mock htmlwidgets::saveWidget using testthat::with_mock
or mockery::stub/mock), and that a file is created when not mocked (use
tempfile() and file.exists); and (2) assert previewNetworkInBrowser calls
htmlwidgets::saveWidget and then utils::browseURL when interactive() is TRUE
(mock interactive, htmlwidgets::saveWidget and utils::browseURL or use
testthat::with_mocked_bindings), verifying the temporary filename is
returned/invisible and browseURL received that filename; target the functions
exportNetworkToHTML, previewNetworkInBrowser, saveWidget and browseURL in the
mocks.
In `@R/utils_cytoscapeNetwork.R`:
- Around line 206-212: The consolidation logic currently builds a key as
paste(e$source, e$target, e$interaction) and assigns consolidated[[key]] <- de,
which causes later directed duplicates to overwrite earlier ones and lose
metadata; update the handling in the consolidation loop that uses variables e,
de and consolidated so directed edges are preserved deterministically by either
(a) aggregating duplicate rows into a single consolidated entry by combining
metadata fields (e.g., evidenceLink, site, ptm_overlap) into lists/vectors when
consolidated[[key]] already exists, or (b) giving each duplicate a unique id
when inserting (e.g., append a counter or original row id to the key) so you set
consolidated[[unique_key]] <- de instead of overwriting; implement one of these
strategies and ensure edge_type is still set to "directed" and ptm_overlap/other
metadata are preserved.
---
Duplicate comments:
In `@R/exportNetworkToHTML.R`:
- Around line 8-15: The function exportNetworkToHTML currently declares an
unused ... argument and has a stale roxygen `@param` ... description mentioning
exportCytoscapeToHTML; remove the unused ... from the function signature and
delete or update the roxygen line that reads "Additional arguments passed to
exportCytoscapeToHTML()", ensuring the documentation accurately reflects the
actual parameters (e.g., nodes, edges, filename, displayLabelType, nodeFontSize)
and return value; keep the `@export` tag and verify no other internal calls rely
on ... in exportNetworkToHTML.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
NAMESPACER/cytoscapeNetwork.RR/exportNetworkToHTML.RR/utils_cytoscapeNetwork.Rman/exportNetworkToHTML.Rdman/previewNetworkInBrowser.Rdtests/testthat/test-utils_cytoscapeNetwork.R
🚧 Files skipped from review as they are similar to previous changes (4)
- man/exportNetworkToHTML.Rd
- man/previewNetworkInBrowser.Rd
- R/cytoscapeNetwork.R
- NAMESPACE
tonywu1999
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/testthat/test-exportNetworkToHTML.R`:
- Around line 66-83: The test currently checks that all labels are members of
c("TP53","MDM2") which allows missing expected labels; update the assertion so
it explicitly verifies the expected labels are present in the captured widget
labels. After constructing widget_arg and computing protein_nodes and labels
(from mock_args(save_widget_mock) and exportNetworkToHTML), replace the subset
assertion with an explicit check that both "TP53" and "MDM2" appear (for example
using expect_setequal or checking that all(c("TP53","MDM2") %in% labels)) so the
test fails if any expected label is missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and context — short summary
The PR refactors the Cytoscape-based HTML export/preview flow to replace a bespoke configuration → JS → standalone-HTML pipeline with an htmlwidgets-based approach. exportNetworkToHTML now constructs a cytoscapeNetwork htmlwidget and saves it via htmlwidgets::saveWidget; previewNetworkInBrowser delegates to that export and opens a temporary file when interactive. This simplifies the codebase, centralizes rendering via cytoscapeNetwork, removes legacy custom HTML/JS generation helpers, and relies on htmlwidgets for persistence and browser preview.
Detailed changes
Unit tests added or modified
Coding guidelines / potential violations